I should follow up since I did notice the answer to one of my questions ... the interface for selecting the convention can't be quite as simple as the prototype lookupSplinePlugSuffixes, since 3delight converts the basis to an array of ints ( though that could just be one extra flag if we're OK with hardcoding things ).

Oh, and right before bed, I finally put 2 and 2 together ... the reason why the shader type isn't round-tripping is probably because I broke that by removing the shader name prefix for USDView compatibility.

Why do we prefix shader types when writing USD? I assume that just deleting this code is not an actual solution, but it does seem to be required if we want to write shaders that are compatible with USDView.

git blame wasn't of much use here, but I think the answer is that we were matching Autodesk's convention for Arnold shaders. We did all our initial testing with Arnold, so that would make sense.

In general, I guess anyone could choose any naming convention, because it's all down to how they register their shaders with RenderMan's Sdr registry? But we're currently working without that registry, to avoid having to ship everyone's USD plugins with Gaffer.

Is it OK to drive this just based on the shader name?

I think so. A quick glance at a mix of C++ and OSL Pxr shaders suggests that they all use the same convention, so I'm not sure we need to care about OSL vs C++, unless you've seen otherwise?

is the registry driven by a shader name prefix?

I'd suggest we use the same key as use for Gaffer's shader metadata registry - <type>:<name> as in ai:surface:standard_surface. Combined with wildcards that would let us register conversions for whole sets of shaders at once, and override for individual shaders.

Maybe all this is a good argument that we shouldn't create a registry yet, and wait until we have more data on what is required before we expose anything in the API.

That's fine with me. As things stand I think I'd be happier with the extra 3Delight-specific flag than I would a more complex mechanism, but a wait-and-see-approach seems wise anyway.

the reason why the shader type isn't round-tripping is probably because I broke that by removing the shader name prefix for USDView compatibility.

I don't think we've ever round-tripped shader type - see https://github.com/GafferHQ/gaffer/blob/main/python/GafferRenderManTest/RenderManShaderTest.py#L104-L134. I think this may be because we've been discussing removing the concept of type (as in surface vs shader etc) entirely, since OSL was moving away from it?

Hey @danieldresser-ie not sure if you saw here but I did some initial testing here if it helps: #1450

What I've learned of the convention that Prman uses for splines is a common match to how Katana is doing them (maybe one influenced the other? Not sure) and KtoA tends to match this convention as well, despite the differently named inputs (and lack of interpolation arrays, it is just a constant remapped to the array). And yep C++ and OSL are 1:1 here as well.

Coming back to this now that the Gaffer side is mostly working and I'm trying to get up more final PRs for both:

Re: Prefixing with shader type:
I think the answer is that we were matching Autodesk's convention for Arnold shaders. We did all our initial testing with Arnold, so that would make sense.

Does this mean that we should only prefix Arnold shaders with "arnold:", and leave off the prefix for everything else? I guess I need to run tests of how Arnold handles osl shaders ( does it require an "osl:" prefix )? Not sure what we'll do if Arnold only supports OSL with an "osl:" prefix, and PRMan only supports OSL without an "osl:" prefix.

I don't think we've ever round-tripped shader type - see https://github.com/GafferHQ/gaffer/blob/main/python/GafferRenderManTest/RenderManShaderTest.py#L104-L134. I think this may be because we've been discussing removing the concept of type (as in surface vs shader etc) entirely, since OSL was moving away from it?

It's true that we've been moving away from the surface vs shader distinction ( that test fails because ri:surface comes back in as ri:shader ). But with the change in this PR to match how the Hydra backend expects PRMan shaders, we lose the renderer prefix as well: "ri:surface" comes back as just "surface". Maybe that's OK too?

Does this mean that we should only prefix Arnold shaders with "arnold:", and leave off the prefix for everything else?

I don't think so - as we discussed yesterday, not changing behaviour for current productions is of prime importance. We should change behaviour for RenderMan shaders only.

But with the change in this PR to match how the Hydra backend expects PRMan shaders, we lose the renderer prefix as well: "ri:surface" comes back as just "surface". Maybe that's OK too?

Are you talking about the shader type here, or the attribute name? We definitely should not change the attribute name. Ideally we'd keep the ri: in the type too.

Updated this for discussion. The ShaderNetworkAlgo commit now deals reasonably with the 4th plug used by the PRMan spline convention.

I've also updated the IECoreUSD::ShaderAlgo commit with a possible way forward: pass in the attribute where we're assigning the shader, so we can skip the type prefix when we're outputting RenderMan shaders. Note that this doesn't help if an OSL shader is directly connected to a shader assignment - that results in assigning to the attribute "osl:surface", so it doesn't trip my check for "ri", and the shaders still get prefixed. But maybe we're "lucky" here, because I've never been able to get RenderMan to render an OSL shader directly - it seems to require an explicit surface shader as the last thing in the chain ( or maybe it would take an OSL shader that returns a closure? No one seems to be using those much? ).

To reply to some of your comments:

Are you talking about the shader type here, or the attribute name?

I had been a bit confused, but this only affects shader type. The attribute name is fine.

Ideally we'd keep the ri: in the type too.

Keeping "ri:" in the type definitely prevents USDView from rendering the shaders through PRMan. Unless you mean just keeping it in the shader type on the Gaffer side, and exploring other ways of storing it while in USD? I guess we could stuff it in some sort of metadata?

Addressed inline comments, added two more commits with tests for IECoreUSD and ShaderNetworkAlgo

Retargeted, squashed ( and took a pass at writing Changelog entries )